Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AP_DDS: Mode Switch Service #24706

Merged
merged 2 commits into from
Sep 10, 2023
Merged

Conversation

arshPratap
Copy link
Member

This PR aims to add Mode Switch service and has a dependency on #24705
@srmainwaring @Ryanf55 @tridge

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Aug 19, 2023

Nice! Where is the mode switch IDL at? I would like to review the message and compare to rep147

@arshPratap
Copy link
Member Author

@Ryanf55 you can find the IDLs currently defined in the yaml file

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Aug 19, 2023

@Ryanf55 you can find the IDLs currently defined in the yaml file

Is there a way to add enums or int constants so the user knows what the modes ID's are?

@srmainwaring
Copy link
Contributor

Where is the mode switch IDL at?

The service interface definition is in ./Tools/ros2/ardupilot_msgs. Perhaps as a follow up PR we could generate the IDL from this same as for other ROS msgs and then update the size and serialisation code?

@arshPratap
Copy link
Member Author

arshPratap commented Aug 28, 2023

@Ryanf55 , @srmainwaring and @tridge have made the changes that were requested..Would like to know your thoughts

@srmainwaring
Copy link
Contributor

srmainwaring commented Aug 28, 2023

  1. Change PR from draft to open as it's ready for review

  2. Update ./libraries/AP_DDS/README.md

https://github.com/arshPratap/ardupilot/blob/4ec66c885446ab5fab7ae294f37aa43654d73826/libraries/AP_DDS/README.md?plain=1#L260

The name of the IS config file has changed to DDS_Service_IS_config.yaml.

  1. Provide an example for the mode switch service. e.g.
ros2 service call /mode_switch ardupilot_msgs/srv/ModeSwitch "{mode: 1}"
requester: making request: ardupilot_msgs.srv.ModeSwitch_Request(mode=1)

response:
ardupilot_msgs.srv.ModeSwitch_Response(status=True, curr_mode=1)

with the expected output on the GSC console:

Mode ACRO
AP: DDS Client: Request for Mode Switch : SUCCESS

@arshPratap
Copy link
Member Author

@srmainwaring done !

  1. Update ./libraries/AP_DDS/README.md

https://github.com/arshPratap/ardupilot/blob/4ec66c885446ab5fab7ae294f37aa43654d73826/libraries/AP_DDS/README.md?plain=1#L260

The name of the IS config file has changed to DDS_Service_IS_config.yaml.

  1. Provide an example for the mode switch service.

@srmainwaring
Copy link
Contributor

srmainwaring commented Aug 28, 2023

Have replicated the examples. FWIW on macOS there are a few extra / more detailed steps needed to build and run.

macOS instructions

Build is-ros2-mix-generator:

colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_MACOSX_RPATH=FALSE -DUAGENT_USE_SYSTEM_FASTDDS=ON -DUAGENT_USE_SYSTEM_FASTCDR=ON -DMIX_ROS_PACKAGES="example_interfaces ardupilot_msgs" --packages-select is-ros2-mix-generator

Run integration-service:

LD_LIBRARY_PATH=$(pwd)/install/is-fastdds/lib ./build/is-core/integration-service ./src/ardupilot/libraries/AP_DDS/Is-Config/DDS_Service_IS_config.yaml --is-prefix-path=$(pwd)/install/is-ros2/lib/is/ros2 --is-prefix-path=$(pwd)/install/is-ros2-mix-generator/lib/is

Where I've placed the integration service package into the colcon workspace for ros2-ardupilot which reduces the need to switch between multiple projects and workspaces.

  • LD_LIBRARY_PATH is needed because the library search paths are filtered out by the macOS shell security settings unless you disable SIP.
  • For similar reasons the IS prefix path needs to be explicitly passed to the integration-service executable otherwise the is-ros2-mix-generator plugins will not be resolved. This is largely because eProsima seem to have implemented their own non-standard plugin resolution system for some reason?

It is worth remarking that there is an integration service build dependency on #24705, as without the updated version of ardupilot_msgs the integration service will not have the interface definitions for the service ardupilot_msgs/srv/ModeSwitch.

@arshPratap arshPratap changed the title Mode Switch Service AP_DDS: Mode Switch Service Aug 28, 2023
@tridge tridge marked this pull request as ready for review August 29, 2023 06:20
@arshPratap arshPratap force-pushed the ddsModeSwitchSrv branch 2 times, most recently from 3eb7c3f to 5294e1b Compare August 31, 2023 13:43
@@ -10,6 +10,7 @@
#include <AP_BattMonitor/AP_BattMonitor.h>
#include <AP_AHRS/AP_AHRS.h>
#include <AP_Arming/AP_Arming.h>
#include <AP_Vehicle/AP_Vehicle.h>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one thing I am unsure about is the circular includes.

Oddly enough, AP_Vehicle.h doesn't include AP_DDS_Client.h.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AP_Vehicle.h forward declares AP_DDS_Client, has a pointer member variable dds_client, and pulls the include into the translation unit. So I think that's ok.

@arshPratap
Copy link
Member Author

@srmainwaring thanks for the work on #24880 .. have rebased this PR over the latest changes..Hoping to have this merged soon.

@srmainwaring
Copy link
Contributor

have rebased this PR over the latest changes.

LGTM @arshPratap. Tested in SITL and working fine.

@peterbarker peterbarker merged commit 8c2627c into ArduPilot:master Sep 10, 2023
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants